fix(miner): governor reputation-history read-modify-write is unguarded, unlike sibling scalar-state writes by phamngocquy · Pull Request #8988 · JSONbored/loopover

phamngocquy · 2026-07-26T15:20:54Z

Summary

packages/loopover-miner/lib/governor-state.ts:382-396's loadReputationHistory/saveReputationHistory pair has no withTransaction wrapper, unlike the other three scalar-state methods in the same file (lines 292-296 explain why those three need it: "so two processes on the same file... cannot interleave a stale read with each other's write"). packages/loopover-miner/lib/loop-cli.ts:580-587 does const prior = governorState.loadReputationHistory(...); governorState.saveReputationHistory(..., { decided: prior.decided + 1, unfavorable: prior.unfavorable + (isRejectedPr ? 1 : 0) }, ...) -- a classic read-modify-write with no transaction around the pair, even though governor-state.sqlite3 is shared across fleet containers (DEPLOYMENT.md:60). Two containers finishing an attempt concurrently can both read the same counts and both +1, losing an increment in the Governor's self-reputation safety throttle.

Deliverables

The reputation-history read-modify-write in governor-state.ts (or its loop-cli.ts call site) is wrapped in a single atomic transaction
A concurrency test simulating two overlapping increments (e.g. two saveReputationHistory calls racing against one loadReputationHistory read) asserting no increment is lost

All of the above Deliverables are required in the same PR unless the deliverable text itself states otherwise.

Test plan

packages/loopover-miner/** -- 99%+ patch coverage including the new concurrency test path.

Fixes #8855

…d, unlike sibling scalar-state writes Fixes JSONbored#8855

superagent-security · 2026-07-26T15:21:24Z

Superagent didn't find any vulnerabilities or security issues in this PR.

codecov · 2026-07-26T15:25:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.68%. Comparing base (f32e140) to head (57656e4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8988      +/-   ##
==========================================
+ Coverage   90.56%   90.68%   +0.12%     
==========================================
  Files          96       98       +2     
  Lines       22490    22889     +399     
  Branches     3884     4017     +133     
==========================================
+ Hits        20367    20757     +390     
- Misses       1945     1950       +5     
- Partials      178      182       +4

Flag	Coverage Δ
backend	`97.74% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/loopover-miner/lib/governor-state.ts	`94.15% <100.00%> (ø)`
packages/loopover-miner/lib/loop-cli.ts	`100.00% <100.00%> (ø)`

loopover-orb · 2026-07-26T15:31:02Z

Tip

✅ LoopOver review result - approve/merge recommended

_{Review updated: 2026-07-26 15:31:21 UTC}

5 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

safe to merge

Review summary
This PR adds `incrementReputationHistory`, a properly transaction-wrapped (BEGIN IMMEDIATE/COMMIT/ROLLBACK) read-modify-write on `governor_reputation_history`, matching the pattern already used by the sibling scalar-state writes in the same file. `loop-cli.ts`'s reentry path is switched from the unguarded load-then-save pair to this single atomic call, correctly closing the race described in the issue. The test suite includes a real regression test that first demonstrates the loss under the old load/save pattern, a same-process serialization test, a cross-process (real child process) concurrency test, a rollback-on-throw test, and a singleton-wrapper round-trip test, which is thorough coverage of the new code path.

Nits — 3 non-blocking

governor-state.ts and loop-cli.ts are both flagged by the size-smell brief as long files, though this diff only adds ~30/-0 and ~2/-6 lines respectively, so it isn't meaningfully worsening that pre-existing condition.
loop-cli.ts:583 still reads `claimed.repoFullName`/`claimed.apiBaseUrl` instead of the already-captured `claimedEntry` used everywhere else in this block, which is a pre-existing inconsistency the diff doesn't need to fix but is worth tightening for readability.
Consider using `claimedEntry.repoFullName`/`claimedEntry.apiBaseUrl` at loop-cli.ts:583-587 for consistency with the rest of the surrounding block, which already captured `claimedEntry` to avoid relying on the loop's reassignable `claimed` variable.

Decision drivers

✅ Code review — No blockers (1 reviewer)
✅ Gate result — Passing (No configured blocker found.)

Context & advisory signals — never blocks the verdict

Signal	Result	Evidence
Linked issue	✅ Linked	#8855
Related work	✅ No active overlap found	No same-issue or scoped active PR overlap found.
Change scope	✅ 20/20	Low review scope from cached public metadata (1 linked issue).
Validation posture	✅ 25/25	PR body includes validation/test evidence.
Contributor workload	✅ 10/10	Author activity: 1 registered-repo PR(s), 0 merged, 0 issue(s).
Contributor context	✅ Confirmed Gittensor contributor	phamngocquy; Gittensor profile; 1 PR(s), 0 issue(s).
Improvement	✅ Minor	risk: clean · value: minor · LLM: significant

Linked issue satisfaction

Addressed
The PR adds an atomic incrementReputationHistory method wrapped in withTransaction, updates loop-cli.ts's call site to use it instead of the unguarded read-modify-write, and includes both in-process and cross-process concurrency tests asserting no increment is lost.

Review context

Author: phamngocquy
Role context: outside_contributor
Public audience mode: oss maintainer
Lane context: Repository is configured for direct PR review.
Public profile languages: Java, Python, Lua, Jupyter Notebook, C, Dockerfile, JavaScript, Shell
Official Gittensor activity: 1 PR(s), 0 issue(s).
PR-specific overlap: none found.

Contributor next steps

Keep the PR focused and include validation evidence before maintainer review.

Signal definitions

Related work = same linked issue, overlapping active PRs, or title/path similarity.
Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

@loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
@loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

_{🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed}

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

Re-run LoopOver review

loopover-orb

LoopOver approves — the gate is satisfied and CI is green.

phamngocquy requested a review from JSONbored as a code owner July 26, 2026 15:20

fix(miner): governor reputation-history read-modify-write is unguarde…

57656e4

…d, unlike sibling scalar-state writes Fixes JSONbored#8855

loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026

loopover-orb Bot assigned phamngocquy Jul 26, 2026

loopover-orb Bot approved these changes Jul 26, 2026

View reviewed changes

loopover-orb Bot merged commit 62e6d25 into JSONbored:main Jul 26, 2026
8 checks passed

github-actions Bot mentioned this pull request Jul 26, 2026

Miner release due: 3.17.0 #8639

Closed

7 tasks

Navigation Menu

Uh oh!

fix(miner): governor reputation-history read-modify-write is unguarded, unlike sibling scalar-state writes - #8988

fix(miner): governor reputation-history read-modify-write is unguarded, unlike sibling scalar-state writes#8988
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-8855

phamngocquy commented Jul 26, 2026 •

edited

Loading

Uh oh!

superagent-security Bot commented Jul 26, 2026

Uh oh!

codecov Bot commented Jul 26, 2026 •

edited

Loading

Uh oh!

loopover-orb Bot commented Jul 26, 2026 •

edited

Loading

✅ LoopOver review result - approve/merge recommended

Uh oh!

loopover-orb Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

phamngocquy commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Deliverables

Test plan

Uh oh!

superagent-security Bot commented Jul 26, 2026

Uh oh!

codecov Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

loopover-orb Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ LoopOver review result - approve/merge recommended

Uh oh!

loopover-orb Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

phamngocquy commented Jul 26, 2026 •

edited

Loading

codecov Bot commented Jul 26, 2026 •

edited

Loading

loopover-orb Bot commented Jul 26, 2026 •

edited

Loading